-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Common api - Destroy and delete queries #15177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR implements a significant architectural refactoring that separates soft delete and hard delete operations across both GraphQL and REST APIs. The changes move shared delete/destroy logic from GraphQL-specific resolvers to common query runner services, creating a unified layer that can be consumed by both API types.
The refactoring introduces clear semantic distinctions: 'delete' operations now perform soft deletes (setting deletedAt timestamps), while 'destroy' operations perform permanent hard deletes. For REST APIs, new handlers are created for soft delete operations (RestApiDeleteOneHandler, RestApiDeleteManyHandler), while existing delete handlers are renamed to destroy handlers (RestApiDestroyOneHandler, RestApiDestroyManyHandler).
The implementation uses feature flags (IS_COMMON_API_ENABLED) to enable gradual rollout of the new common API services while maintaining backward compatibility. GraphQL resolvers now conditionally use either the new common services or fall back to existing implementations. The changes also include comprehensive OpenAPI documentation updates to support the new soft delete functionality through query parameters.
This consolidation reduces code duplication, improves consistency between API types, and provides clearer semantics for different deletion behaviors. The common query runner pattern ensures both GraphQL and REST APIs share the same underlying business logic for delete and destroy operations.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
packages/twenty-server/src/engine/api/common/common-query-runners/common-destroy-one-query-runner.service.ts |
4/5 | New service for hard deleting single records, delegates to destroy many service |
packages/twenty-server/src/engine/api/common/common-query-runners/common-destroy-many-query-runner.service.ts |
4/5 | New service for hard deleting multiple records, extracted from GraphQL resolvers |
packages/twenty-server/src/engine/api/common/common-query-runners/common-delete-one-query-runner.service.ts |
4/5 | New service for soft deleting single records, delegates to delete many service |
packages/twenty-server/src/engine/api/common/common-query-runners/common-delete-many-query-runner.service.ts |
4/5 | New service for soft deleting multiple records, uses TypeORM softDelete method |
packages/twenty-server/src/engine/api/rest/core/services/rest-api-core.service.ts |
4/5 | Major refactor adding complex routing logic for delete vs destroy operations |
packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-delete-one.handler.ts |
4/5 | Completely refactored to use common service for soft delete operations |
packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-destroy-one.handler.ts |
3/5 | New handler for hard delete operations with duplicate method implementations |
packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-destroy-many.handler.ts |
4/5 | New handler for hard deleting multiple records via REST |
packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-delete-many.handler.ts |
4/5 | New handler for soft deleting multiple records via REST |
packages/twenty-server/src/engine/api/graphql/workspace-resolver-builder/factories/destroy-one-resolver.factory.ts |
4/5 | Updated to conditionally use common API service with feature flag |
packages/twenty-server/src/engine/api/graphql/workspace-resolver-builder/factories/destroy-many-resolver.factory.ts |
3/5 | Updated with feature flag but has hardcoded pagination values |
packages/twenty-server/src/engine/api/graphql/workspace-resolver-builder/factories/delete-one-resolver.factory.ts |
4/5 | Updated to conditionally use common API service with feature flag |
packages/twenty-server/src/engine/api/graphql/workspace-resolver-builder/factories/delete-many-resolver.factory.ts |
4/5 | Updated to conditionally use common API service with feature flag |
packages/twenty-server/src/engine/core-modules/open-api/utils/parameters.utils.ts |
5/5 | Added new function for soft delete parameter documentation |
packages/twenty-server/src/engine/core-modules/open-api/utils/responses.utils.ts |
4/5 | Added new function for delete many response schema generation |
packages/twenty-server/src/engine/core-modules/open-api/utils/path.utils.ts |
4/5 | Updated OpenAPI paths to support soft delete functionality |
packages/twenty-server/src/engine/core-modules/open-api/utils/components.utils.ts |
5/5 | Added soft delete parameter to OpenAPI components |
packages/twenty-server/src/engine/api/rest/core/rest-api-core.module.ts |
4/5 | Added new handler imports and reorganized resolver array |
packages/twenty-server/src/engine/api/rest/input-request-parsers/soft-delete-parser-utils/parse-soft-delete-rest-request.util.ts |
5/5 | New utility to parse soft delete query parameter from requests |
packages/twenty-server/src/engine/api/common/types/common-query-args.type.ts |
4/5 | Added new query types and interfaces for delete/destroy operations |
packages/twenty-server/src/engine/api/common/common-query-runners/common-query-runners.ts |
5/5 | Added exports for new delete and destroy query runner services |
packages/twenty-server/src/engine/api/rest/core/interfaces/rest-api-base.handler.ts |
4/5 | Made depth parameter optional in computeSelectedFields method |
packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-one.handler.ts |
4/5 | Updated response format to include operation prefix using capitalize utility |
packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-many.handler.ts |
4/5 | Updated response format and added dual method approach with common service |
Confidence score: 3/5
- This PR introduces complex changes with potential breaking effects and inconsistencies that need careful review
- Score reflects concerns about duplicate code paths, naming inconsistencies, hardcoded values, and complex conditional logic that could lead to maintenance issues
- Pay close attention to REST handlers, GraphQL resolvers with feature flags, and the service routing logic in
rest-api-core.service.ts
Sequence Diagram
sequenceDiagram
participant User
participant RestController as "REST API Controller"
participant RestService as "REST API Core Service"
participant DeleteHandler as "Delete Handler"
participant DestroyHandler as "Destroy Handler"
participant CommonRunner as "Common Query Runner"
participant QueryBuilder as "Query Builder"
participant Database as "Database"
User->>RestController: "DELETE /api/rest/objects/{id}?soft_delete=true"
RestController->>RestService: "delete(request)"
alt Feature Flag Enabled & Soft Delete
RestService->>DeleteHandler: "handle(request)"
DeleteHandler->>CommonRunner: "run({ args, authContext })"
CommonRunner->>QueryBuilder: "createQueryBuilder()"
CommonRunner->>QueryBuilder: "softDelete().returning(columns)"
QueryBuilder->>Database: "UPDATE SET deletedAt = NOW()"
Database-->>QueryBuilder: "Soft deleted records"
QueryBuilder-->>CommonRunner: "Deleted records"
CommonRunner-->>DeleteHandler: "Records with deletedAt set"
DeleteHandler-->>RestService: "Formatted response with softDelete: true"
else Feature Flag Enabled & Hard Delete
RestService->>DestroyHandler: "handle(request)"
DestroyHandler->>CommonRunner: "run({ args, authContext })"
CommonRunner->>QueryBuilder: "createQueryBuilder()"
CommonRunner->>QueryBuilder: "delete().returning(columns)"
QueryBuilder->>Database: "DELETE FROM table"
Database-->>QueryBuilder: "Permanently deleted records"
QueryBuilder-->>CommonRunner: "Deleted records"
CommonRunner-->>DestroyHandler: "Records removed from database"
DestroyHandler-->>RestService: "Formatted response"
end
RestService-->>RestController: "Deletion result"
RestController-->>User: "HTTP 200 with deleted record data"
Additional Comments (2)
-
packages/twenty-server/src/engine/api/common/types/common-query-args.type.ts, line 67-69 (link)style: Remove trailing empty lines for consistency with codebase formatting
-
packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-create-many.handler.ts, line 76-162 (link)style: Legacy handle method maintained alongside new commonHandle - this creates dual code paths that may lead to maintenance issues.
Context used (9)
- Rule from
dashboard- Method names should be descriptive and specific. Use 'getScopesFromGoogleAccessToken' instead of 'ge... (source) - Rule from
dashboard- Extract utility functions like 'includesExpectedScopes' to utils modules instead of keeping them in ... (source) - Rule from
dashboard- Error messages should be user-friendly rather than technical. Avoid technical jargon like 'missing s... (source) - Context from
dashboard- Follow the convention of having only one utility function per exported file in the project. (source) - Context from
dashboard- Ensure that type assertions are safe and consider using type guards instead of direct assertions. (source) - Context from
dashboard- Implement real validation for the REST API to ensure that errors are accurately categorized and not ... (source) - Context from
dashboard- Always consider adding tests for new functionality, especially for edge cases like empty responses. (source) - Context from
dashboard- Ensure that any transformation logic related to enums is implemented at a global level rather than w... (source) - Context from
dashboard- Use 'type' instead of 'interface' for type definitions in the project. (source)
24 files reviewed, 23 comments
packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-destroy-many.handler.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-delete-many.handler.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/open-api/utils/responses.utils.ts
Outdated
Show resolved
Hide resolved
...erver/src/engine/api/common/common-query-runners/common-destroy-many-query-runner.service.ts
Outdated
Show resolved
Hide resolved
...erver/src/engine/api/common/common-query-runners/common-destroy-many-query-runner.service.ts
Outdated
Show resolved
Hide resolved
...server/src/engine/api/common/common-query-runners/common-delete-many-query-runner.service.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/api/rest/core/services/rest-api-core.service.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/api/rest/core/services/rest-api-core.service.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/api/rest/core/services/rest-api-core.service.ts
Show resolved
Hide resolved
.../src/engine/api/graphql/workspace-resolver-builder/factories/delete-many-resolver.factory.ts
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:15154 This environment will automatically shut down when the PR is closed or after 5 hours. |
183d0ab to
3751479
Compare
| const queryBuilder = repository.createQueryBuilder( | ||
| objectMetadataItemWithFieldMaps.nameSingular, | ||
| ); | ||
|
|
||
| const tableName = computeTableName( | ||
| objectMetadataItemWithFieldMaps.nameSingular, | ||
| objectMetadataItemWithFieldMaps.isCustom, | ||
| ); | ||
|
|
||
| commonQueryParser.applyFilterToBuilder( | ||
| queryBuilder, | ||
| tableName, | ||
| processedArgs.filter, | ||
| ); | ||
|
|
||
| const columnsToReturn = buildColumnsToReturn({ | ||
| select: selectedFieldsResult.select, | ||
| relations: selectedFieldsResult.relations, | ||
| objectMetadataItemWithFieldMaps, | ||
| objectMetadataMaps, | ||
| }); | ||
|
|
||
| const deletedObjectRecords = await queryBuilder | ||
| .softDelete() | ||
| .returning(columnsToReturn) | ||
| .execute(); | ||
|
|
||
| const deletedRecords = deletedObjectRecords.generatedMaps as ObjectRecord[]; | ||
|
|
||
| await this.processNestedRelationsIfNeeded({ | ||
| records: deletedRecords, | ||
| objectMetadataItemWithFieldMaps, | ||
| objectMetadataMaps, | ||
| rolePermissionConfig, | ||
| authContext, | ||
| workspaceDataSource, | ||
| selectedFieldsResult, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic c/p from packages/twenty-server/src/engine/api/graphql/graphql-query-runner/resolvers/graphql-query-delete-many-resolver.service.ts
| this.validate(args, objectMetadataItemWithFieldMaps); | ||
|
|
||
| if (!isWorkspaceAuthContext(authContext)) { | ||
| throw new CommonQueryRunnerException( | ||
| 'Invalid auth context', | ||
| CommonQueryRunnerExceptionCode.INVALID_AUTH_CONTEXT, | ||
| ); | ||
| } | ||
|
|
||
| const { workspaceDataSource, repository, rolePermissionConfig } = | ||
| await this.prepareQueryRunnerContext({ | ||
| authContext, | ||
| objectMetadataItemWithFieldMaps, | ||
| }); | ||
|
|
||
| const commonQueryParser = new GraphqlQueryParser( | ||
| objectMetadataItemWithFieldMaps, | ||
| objectMetadataMaps, | ||
| ); | ||
|
|
||
| const selectedFieldsResult = commonQueryParser.parseSelectedFields( | ||
| objectMetadataItemWithFieldMaps, | ||
| args.selectedFields, | ||
| objectMetadataMaps, | ||
| ); | ||
|
|
||
| const processedArgs = await this.processQueryArgs({ | ||
| authContext, | ||
| objectMetadataItemWithFieldMaps, | ||
| args, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no logic modification but reformatted. from base resolver, before calling resolver service
| const enrichedRecords = await this.enrichResultsWithGettersAndHooks({ | ||
| results: deletedRecords, | ||
| operationName: CommonQueryNames.deleteMany, | ||
| authContext, | ||
| objectMetadataItemWithFieldMaps, | ||
| objectMetadataMaps, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no logic modification but reformatted. from base resolver, after calling resolver service
| @@ -0,0 +1,198 @@ | |||
| import { Injectable } from '@nestjs/common'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no logic creation in this file
| @@ -0,0 +1,68 @@ | |||
| import { Injectable } from '@nestjs/common'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no logic creation in this file
| @@ -0,0 +1,182 @@ | |||
| import { Injectable } from '@nestjs/common'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no logic creation in this file
| @@ -0,0 +1,66 @@ | |||
| import { Injectable } from '@nestjs/common'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no logic creation in this file
| Injectable, | ||
| InternalServerErrorException, | ||
| } from '@nestjs/common'; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix, should have be done in other PRs
| Injectable, | ||
| InternalServerErrorException, | ||
| } from '@nestjs/common'; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix, should have be done in other PRs
3751479 to
a1d207f
Compare
397ed4f to
0cd7489
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments, but looks good overall, especially with this new pattern !
Done ⬇️
Gql : move delete and destroy logic to common
Rest :
--> Rest api gains NEW soft delete one/many + destroy many capabilities
closes : twentyhq/core-team-issues#1579